-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove unneeded message on command completion. Set error output to Stderr #432
Remove unneeded message on command completion. Set error output to Stderr #432
Conversation
Signed-off-by: Víctor Martínez Bevià <[email protected]>
Hey @vicmarbev the removal of Succeed text might have some impact on the tools that consume imgpkg as a binary, like vendir or even imgpkg. Is there any particular reason for you to want to remove it? When you run |
Signed-off-by: Víctor Martínez Bevià <[email protected]>
What about that last commit? It's the same approach they used in Kapp about this issue: carvel-dev/kapp#592 |
some work is happening in kapp to clean up similar functionality. will want to port over those changes here. stay tuned. |
updated kapp change: carvel-dev/kapp@ed8c3f2 |
Hi @cppforlife could you help me with the |
I was looking at the PR and to be fair I am not sure I agree with the name of the function that was added to cobrautil. |
Yeah, I agree with you on this. |
I think what we want is maybe a function that let us know that we should not provide extra output to them because in some sense this is the only use of this particular function. |
💯
@vicmarbev I don't see any diff in the vendor directory, you might wanna run |
I think the problem might be as simple as the vendored packaged was not added to the commit. So if there is a change in the vendor we need to dad the file and commit it. If this does not solve the problem I can take a look at it later |
That did it. Thanks! |
Anything else needed for this PR? |
@vicmarbev I am waiting on the PR cppforlife/cobrautil#6 to get merged and when that is done I would ask you to bump the cobrautil library and I think we should be ready to go |
@vicmarbev whenever you can please bump cobrautil to the latest version so that we can use the new function. After that I think we will be ready to merge this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #426
Basically I removed the "Succeeded" message and redirect error output to Stderr. Do you see any problem with this approach? Is there any case that we want to print that "Succeeded" message?
Signed-off-by: Víctor Martínez Bevià [email protected]